Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes behavior of stc.save() as mentioned in issue #13158 #13165

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fixes behavior of stc.save() as mentioned in issue #13158 #13165

wants to merge 9 commits into from

Conversation

shresth-keshari
Copy link
Contributor

@shresth-keshari shresth-keshari commented Mar 18, 2025 •
edited
Loading

Fixes #13158 .

Doing stc.save("test.h5") saves a single test.h5 file, and doing stc.save("test.stc") saves to the two files test-lh.stc and test-rh.stc.

Please let me know if any more changes required in the current iteration.

drammock
drammock reviewed Mar 18, 2025
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI errors look related:

https://github.com/mne-tools/mne-python/actions/runs/13924372063/job/38964972043?pr=13165#step:16:6285

FAILED mne/tests/test_source_estimate.py::test_io_stc - OSError: SourceEstimate File(s) not found for: PosixPath('/private/var/folders/0j/bwqcs4y508s2n4ck4dhf3rpc0000gn/T/pytest-of-runner/pytest-0/test_io_stc0/tmp.stc')

Does mne/tests/test_source_estimate.py::test_io_stc pass for you locally?

@shresth-keshari
Copy link
Contributor Author

@drammock I have changed ftype from None to auto coz it was fairly easy.
I also put it to test by saving a sample of both .stc as well as .h5 filetypes. You can see them in the screenshot, that they're being saved in the directory, named as it was suggested by @larsoner in the issue. (will be deleting these files before commit)

Screenshot from 2025-03-18 22-39-54

But, while running tests, it shows the following error:

image

which says: FAILED test_source_estimate.py::test_io_stc - OSError: SourceEstimate File(s) not found for: PosixPath('/tmp/pytest-of-shresth/pytest-1/test_io_stc0/tmp.stc')

I'm not aware about how to fix this error. Please help.

hoechenberger
hoechenberger requested changes Mar 21, 2025
View reviewed changes
mne/source_estimate.py
@@ -1884,7 +1884,7 @@ class SourceEstimate(_BaseSurfaceSourceEstimate):
"""

@verbose
def save(self, fname, ftype="stc", *, overwrite=False, verbose=None):
def save(self, fname, ftype="auto", overwrite=False, verbose=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove the *

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for pointing out,
I had noticed that a couple of days back, added "*" as suggested, but still, when I run pytest, it fails with the same message as mentioned above.
Well, if I test the function using a sample stc file, it works perfectly.
I guess the problem is not limited only to this, and needs some other tiny fix.

drammock
drammock reviewed Mar 21, 2025
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had noticed that a couple of days back, added "*" as suggested, but still, when I run pytest, it fails with the same message as mentioned above.

The * is not related to the pytest failures

Well, if I test the function using a sample stc file, it works perfectly.

Our tests often check for "corner cases" that are probably different than the approach you're trying. I suggest running pytest with the --pdb flag so you can examine what the problematic values are when the test fails, and see if that helps you figure out what else needs to be changed.

@shresth-keshari
Copy link
Contributor Author

Hey, @drammock, I implemented your suggestions, and ran pytest with --pdb flag.
The issue seems to be coming from "read_source_estimate" in source_estimate.py, which looks like this in the terminal:

test_source_estimate.py:484: in test_io_stc
    stc2 = read_source_estimate(tmp_path / "tmp.stc")

..\source_estimate.py:330: in read_source_estimate
    raise OSError(f"SourceEstimate File(s) not found for: {fname_arg!r}")

E   OSError: SourceEstimate File(s) not found for: WindowsPath('C:/Users/kesha/AppData/Local/Temp/pytest-of-kesha/pytest-3/test_io_stc0/tmp.stc')

Now that I look at test_source_estimate.py, in the function definition of test_io_stc, the stc file is saved first, using stc.save() function. Later, the same file is being read using read _source_estimate. Since we changed the naming of the files being saved, I believe that's what causes the issue. Will try fixing that and will get back to you with a solution soon.

have attached screenshots as well, please have a look:

image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock left review comments

@hoechenberger hoechenberger hoechenberger requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Bugs with stc.save behavior
3 participants
@shresth-keshari @drammock @hoechenberger